-
Notifications
You must be signed in to change notification settings - Fork 653
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(plugins/dates): support duration ranges #1137
feat(plugins/dates): support duration ranges #1137
Conversation
1dfce21
to
1351468
Compare
81d018b
to
2178719
Compare
if (!['day', 'days'].includes(duration)) { | ||
end = end.applyShift({ day: -1 }).applyShift({ [duration]: 1 }) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully this is an okay way of doing this.
I found that days was projecting inclusively, but, other durations weren't.
For example 1-2 weeks
was returned as 8 days rather than 14 days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks @spencermountain!
Hopefully last question, do i need to do anything extra for handling past tense in this?
I.e. 2 to 3 weeks ago
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, no problem - i would make a new section, in addition to this '2 to 4 weeks' one, with similar logic.
go for it!
oh, one thing - maybe add a '^in'
on the match, so it doesn't interfere with 2 to 3 weeks after june 3rd
etc.
lookin good! thanks for the help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cheers @spencermountain, have added the ^in
as requested for the future tense match block.
Struggling with the past tense, as you can see from the now failing tests! When you get a spare second could you take a look at the past tense block please? Thought it'd be as simple as flipping the logic.
If it's not a simple fix, happy to just tear out past tense support for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure - i can merge this to dev and fix the failing tests, if you're ready?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey Frazer, haven't forgotten about this - been really busy, then had covid. Will try to get to it shortly.
cheers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this on @spencermountain, 42f9cd0 appears to have stopped the ranges from being inclusive?
73f52c1
to
3adf437
Compare
3adf437
to
fd45122
Compare
Closes #1135
This will need a squash PR as it's bound to be a mess.